This is not technically exhaustive.
Any 2 shreds with different but overlapping erasure sets can be considered duplicate.
In particular doesn't even need to have the same fec_set_index.

Technically the overlapping erasure sets might result in non-duplicate shreds, but a leader shouldn't generate overlapping erasure sets unless malicious or buggy.

We don't currently identify those cases because we just lookup by the erasure set:
https://github.com/solana-labs/solana/blob/5de27d9a0/ledger/src/blockstore.rs#L1197-L1206
but I think that has to be fixed at some point.

Good to know, right now i've only replicated the cases that we catch in blockstore.
I created #33037, and will extend the checks to catch these types of conflicts in a follow up pr.

Can you put a comment reference to #33037 here?

Is there a use to this any more?
Can we just change this to _unused?

Can you keep this enum sorted?

What if both are last_in_slot but ix1 != ix2?

It will get caught by the _ => () case as this is a valid last index conflict proof.

This part (and now the function as a whole) has become too confusing.
We want to check if the shreds indeed indicate a duplicate slot.
So we need to check that the shreds have the same slot and type and then either of these conditions hold:

• same index, different payload.
• one has last_in_slot but the other has higher index.
• the erasure configs are conflicting.

It is more readable to check if these conditions hold than check if one of them does not hold.

Just rename this to _unused.
You will need to update frozen-abi digest in a couple of places but that is fine.
Also please don't add unnecessary blank lines:
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

If shred1.index() != shred2.index() then the payload cannot be the same.
The index is encoded in the payload.
So the first check is unnecessary.

Can you put a comment reference to #33037 here?

This part (and now the function as a whole) has become too confusing.
We want to check if the shreds indeed indicate a duplicate slot.
So we need to check that the shreds have the same slot and type and then either of these conditions hold:

• same index, different payload.
• one has last_in_slot but the other has higher index.
• the erasure configs are conflicting.

It is more readable to check if these conditions hold than check if one of them does not hold.

assert_matches

matches by itself does not do any assertions here.

same here

To minimize the public api surface, instead of making these 2 functions public, can you add a method which takes two shreds and returns false if their erasure configs mismatch.
That function can call these 2 functions here internally.

This would be more readable if you avoid nested branches:

if blockstore.has_duplicate_shreds_in_slot(shred_slot) {
    return Ok(()); // A duplicate is already recorded.
}
let Some(existing_shred_payload) = blockstore.is_shred_duplicate(&shred) else {
    return Ok(()); // Not a duplicate.
}
blockstore.store_duplicate_slot(
    shred_slot,
    existing_shred_payload.clone(),
    shred.clone().into_payload(),
)?;
(shred, existing_shred_payload)

When you start the field name with underscore, don't need #[allow(dead_code)] anymore.

thanks for the review! I've removed the #[allow(dead_code)]